[multiple-commpaths] Allows colon-separated commpath list - #196
Conversation
2d223fe to
8c7c63f
Compare
Replaces --commpath with --commpaths. Takes a colon-separated list of commpaths. The choice of which of those to use is made per-server. The first workable path is selected. The paths may differ from server to server: unlike --cachepaths, there is no consensus process.
b184a51 to
1072473
Compare
|
@rountree - Are you working on fixing the CI failures? |
07e1a40 to
1072473
Compare
|
@mplegendre Yes. What's queued up for this coming Friday is:
|
See ci-artifacts branch/PR for description. The workflows need be added here as the files are required to be both in the default branch as well as the branch using the workflows.
44a204f to
bd856fa
Compare
|
Debugging notes: Running Running Running Running Running At this point GitHub actions became unusably slow. The good news is that the podman port can replicate the problem on rzadams with a 4% failure rate. |
|
|
||
| static char* exitSocketPath(spindle_args_t *params) | ||
| { | ||
| char *realized_dir, *socket_path = NULL; |
There was a problem hiding this comment.
| char *realized_dir = NULL, *socket_path = NULL; |
With the change below, if getFirstValidPath returns -1, we goto done without ever assigning to realized_dir; before these changes, we would always assign to realized_dir. Since the done block checks the value of realized_dir, we need to initialize it to NULL.
| snprintf(options_str, 32, "%lu", (unsigned long) params->opts); | ||
| string options(options_str); | ||
|
|
||
| getFirstValidPath( params->commpaths, &( params->commpath ), params->number ); |
There was a problem hiding this comment.
Need to handle the case where getFirstValidPath returns -1
| #if !defined(COMMPATH) | ||
| #error COMMPATH must be defined in config.h | ||
| #endif | ||
| #define SESSIONPATH "/tmp/spindle/session" |
There was a problem hiding this comment.
I'm not sure we should hard-code a path for the session directory
| return NULL; | ||
| } | ||
|
|
||
| result = spindle_mkdir(dir); |
There was a problem hiding this comment.
The old code called spindle_mkdir to ensure that the session directory exists, but the new code doesn't. Do we still need the spindle_mkdir?
| [AS_HELP_STRING([--with-compath=DIR],[Back-end directory for communication and housekeeping])], | ||
| [COMMPATH=${withval}], | ||
| [COMMPATH=$DEFAULT_LOC]) | ||
| AC_ARG_WITH(commpaths, |
There was a problem hiding this comment.
We might want to still accept the old commpath (singular) form so that existing configure arguments continue to work
There was a problem hiding this comment.
The above line increased the size of the buffer, but this line still null-terminates at the original size; is that correct?
There was a problem hiding this comment.
Looks like Matt doubled the buffer length. Git blame says:
68bacdf8 src/client/client_comlib/client_api.c (Matthew LeGendre 2026-06-22 11:17:35 -0700 45) char buffer[2*(MAX_PATH_LEN+1)];
af4cd951 src/client/client_comlib/client_api.c (Barry 2025-10-03 09:52:29 -0700 46) buffer[MAX_PATH_LEN] = '\0';
The buffer is only used for the reply message (LDCS_MSG_CHOSEN_CACHEPATH) which copies two directory names into the buffer for a total length of strlen(procdata->cachepath) + 1 + strlen(procdata->parsed_cachepath) + 1. There's no check on the sending side if that value is <= 2*(MAX_PATH_LEN+1), although there may be an implied check upstream.
The line adding the null terminator is not doing anything useful. Adding one to the end of buffer might prevent a segfault if the incoming strings were malformed but otherwise fit into the buffer.
On my own projects I use calloc() instead of malloc() unless there's a good reason not to. Your preference?
For the slurm-plugin case, fix copy/paste error by updating container names to those found in docker-compose.yml. (h/t Nick) Co-authored-by: Nicholas Chaimov <nchaimov@gmail.com>
Typo fix. h/t Nick. Co-authored-by: Nicholas Chaimov <nchaimov@gmail.com>
4 participants
Per-server selection, first path in
--commpathsthat works for that server gets picked. No consensus process, as the commpath is not visible outside of the server and its particular clients.